-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: streamline mayBeNullExpr flow #753
Conversation
9cb0c44
to
8618248
Compare
Thanks for looking into this! Re: a) I put up a commit that shows the problem: There, I changed the order of dataflow and handlers for method calls, and you can see that several tests fail. This is because we rely on dataflow to "override" results from the handler, e.g., in the case of a null check of a call to msridhar@69cea80#diff-f5f114bb393be97fed48630313d15457ba3cbd039ad291304bbbb71245cf6c2aR146 Oddly, In general, I prefer a refactor like what is in the draft here to #748. Now that we've dug into this and I've swapped things in, I'd rather do things right and make the invariants and expectations clear if we're going to do a refactor. |
i was aware that see my suggestion around this in #754 as for the "order problem" compare current master here versus there if we always want dataflow to run after handler overrides, then yeah we can streamline as suggested in this PR |
05cc788
to
f34cd53
Compare
break; | ||
default: | ||
// match switch expressions by comparing strings, so the code compiles on JDK versions < 12 | ||
if (expr.getKind().name().equals("SWITCH_EXPRESSION")) { | ||
exprMayBeNull = nullnessFromDataflow(state, expr); | ||
exprMayBeNull = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that here and above, we used to first run dataflow and only then called the handler overrides
while now we would be doing it the other way round.
running dataflow last seems like the most uniform way?
also there are no tests failing so maybe the old order did not matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed, running dataflow last uniformly is what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do, under some circumstances, want handlers to override the results from the dataflow analysis, but I think this should in all cases be done by using one of the handler methods that run as part of the dataflow analysis itself, not the AST-node taking methods... (Edit: to clarify, I am agreeing with the uniform ordering here)
the uniform flow is now: - return early for obvious expression types - determine initial exprMayBeNull through simple checks - apply handler overrides in single place - finally run dataflow if exprMayBeNull is true in single place
f34cd53
to
3127f16
Compare
} else { | ||
throw new RuntimeException( | ||
"whoops, better handle " + expr.getKind() + " " + state.getSourceForNode(expr)); | ||
} | ||
} | ||
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, exprMayBeNull); | ||
return exprMayBeNull; | ||
return exprMayBeNull && nullnessFromDataflow(state, expr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
together with the LibraryModelsHandler
refactoring, this would be the only place where nullnessFromDataflow
would be getting called still
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this change looks really good to me! Modulo my relatively minor comment, I think we can shift this off of draft mode and land it (though @lazaroclapp you may want to look)
// Check handler.onOverrideMayBeNullExpr before dataflow. | ||
exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, exprSymbol, state, true); | ||
return exprMayBeNull ? nullnessFromDataflow(state, expr) : false; | ||
exprMayBeNull = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here saying we rely on inference / dataflow analysis for local variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, please check if it is as you had mind.
also i tried putting answers to the questions in the PR description, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will take another look later in the day and then likely land this
break; | ||
default: | ||
// match switch expressions by comparing strings, so the code compiles on JDK versions < 12 | ||
if (expr.getKind().name().equals("SWITCH_EXPRESSION")) { | ||
exprMayBeNull = nullnessFromDataflow(state, expr); | ||
exprMayBeNull = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed, running dataflow last uniformly is what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me and thanks for the clean up! Once this and the LibraryModelsHandler
change both land, I'd vote we do some internal sanity testing and also some performance testing. I think we cache dataflow results, so probably we won't see a difference from the reduced number of calls to it, but might be worth double-checking! Either way, the logic is clearer, I feel!
This reverts commit 67c357e.
This reverts commit 67c357e.
This reverts commit 67c357e.
This reverts commit 67c357e.
This reverts commit 67c357e.
idea from discussion on other PR
open questions:
a) why are no tests failing when we return early from the switch for some expressions?
(this is giving performance benefits because we are not redundantly calling the handler chain or dataflow analysis)
-> no handler ever overrides those expression types and dataflow analysis does not matter for them?
b) why are no tests failing because we change the order of "dataflow" and "handler overrides" in some methods, even though they state the order difference is intentional and important there
-> order should be uniform with dataflow running last